#105 Allow Procs for dynamic value usage #135

codegeek319 · 2021-10-06T15:00:02Z

This PR allows Procs for the value determinations at runtime. Also the Test and the Documentation is updated according to the latest code.
closes #105

igorkasyanchuk · 2021-10-06T16:18:01Z

lib/active_storage_validations/content_type_validator.rb

@@ -31,7 +35,7 @@ def types
      end
    end

-    def types_to_human_format
+    def types_to_human_format(types)


@codegeek319 probably naming should be changed, or code refactored

because we have method "types", and we have parameter types and local variables.

this is confusing. Do we need to pass types everywhere?

@igorkasyanchuk sorry for the late answer and tackling of this issue!
For the dynamic proc evaluation, we need to check the options for procs on every validation, call them, then continue the validations with the new options. As the options are used in the types method to determine the authorized types, we now have to pass the new options to it and the resulting authorized types can be different on each validation - that's why we then pass the types on to the methods which need them.

I renamed the types method to authorized_types to avoid confusion with variables and parameters. Looks clearer now, I agree. :)

gr8bit · 2022-02-04T22:46:31Z

@igorkasyanchuk would you mind to take another look? Thanks! :)

igorkasyanchuk · 2022-02-06T21:44:40Z

Hello @gr8bit ,

Sorry for taking so long time, I think I've some important comments now.
I see in the code that often you use something like range = range.call(record) if range.is_a?(Proc) and you checking in many places objects if it's proc or now. Because of it you changed lot's of code, and added some complexity to existing method, for example by adding new parameters.

I've the following idea, it probably can take not long time to implement it, at least I want you to think if it could work better.
So the idea is the following - validation class receives some "options", which are consist of hash and potentially hashes inside.

We can just convert all hashes to values before. So no need to call proc somewhere inside validator. Everything will keep working as before.

I did some quick prototype, you can use it as a reference and improve. Ready to discuss it with you, If needed in skуpe "igorkasyanchuk".

igorkasyanchuk · 2022-02-06T21:48:52Z

@codegeek319 sorry mentioning you too. Just in case you need:

    def recur_call_hash(record, h)
      hash = h.dup
      return hash unless hash.is_a?(Hash)

      puts hash
      hash.each do |(k,v)|
        if v.is_a?(Proc)
          hash[k] = v.call(record)
        else
          hash[k] = v
        end
        if hash[k].is_a?(Hash)
          hash[k] = recur_call_hash(record, hash[k])
        end
      end
      hash
    end

igorkasyanchuk · 2022-02-06T21:50:16Z

and last suggestion, to make in Readme example more close to real life if possible. For example

validates :proc_files, limit: { max: -> (record) { record.admin? ? 100 : 10 } }

gr8bit · 2022-02-06T21:51:33Z

Thank you for the great feedback!! @codegeek319 has been implementing this and we'll discuss it on Tuesday, we work together irl. :)

igorkasyanchuk · 2022-04-17T10:55:33Z

@gr8bit @codegeek319 do you have plans to fix this PR?

gr8bit · 2022-04-17T11:10:36Z

Sure thing! Sorry, we have been quite busy ☺️

…ons into allow_procs * 'master' of github.com:bichinger/active_storage_validations: (32 commits) Create zh-CN.yml Update README.md Update README.md preparation for a release Add test for Dimension validation errors Display All Validation Errors Update README.md rm .ruby-version Fixes Fix tests Update metadata.rb Update metadata.rb Dynamic exception_class Update README.md Fix a file extension issue in Metadata#read_image prepeation for a release prepeation for a release preparing for a release Test against Rails 7.0 and Ruby 3.1 Remove references to image/jpg content type ... # Conflicts: # lib/active_storage_validations/aspect_ratio_validator.rb # lib/active_storage_validations/dimension_validator.rb # test/dummy/app/models/only_image.rb

gr8bit · 2022-09-10T19:00:08Z

@igorkasyanchuk finally I found the time for the refactoring you suggested. Diff shrunk significantly. Would you mind taking another look? :)

igorkasyanchuk · 2022-09-11T18:04:44Z

thank you for your PR, from my side I think it looks good

But I want to check with other developers who did the most contributions to the gem

@StefSchenkelaars , @phlegx , @tleneveu , @reckerswartz what do you think about this PR?

StefSchenkelaars

I really like the idea! Makes it more similar to Active Model so that's good.
Code wise I think it should also be a bit more like Active Model. Especially the local version of the options variable seems dangerous to me since it gives quite some ambiguity.
So try to ensure the instance methods are actually instance methods and are not used as some helper method which still accesses instance variables.
If you have any questions about my comments or need a bit of help implementing, let me know!

StefSchenkelaars · 2022-09-12T11:22:16Z

lib/active_storage_validations/aspect_ratio_validator.rb

@@ -24,11 +26,12 @@ def validate_each(record, attribute, _value)
        changes = record.attachment_changes[attribute.to_s]
        return true if changes.blank?

+        options = unfold_procs(record, self.options, AVAILABLE_CHECKS)


I don't like changing the original options variable. The options variable is made available in the ActiveModel::Validator and I would really keep it that way.

I hope I understand you correctly but the original options variable will not be changed, it will be masked by a local variable called options. To avoid this confusion, I'll rename the local variable to flat_options.

StefSchenkelaars · 2022-09-12T11:23:15Z

lib/active_storage_validations/aspect_ratio_validator.rb

@@ -4,6 +4,8 @@

 module ActiveStorageValidations
  class AspectRatioValidator < ActiveModel::EachValidator # :nodoc
+    include OptionProcUnfolding


Can we reuse the code and logic from active model? Since we depend on it anyway, why not use that a bit more. For example the https://github.com/rails/rails/blob/d695972025146713fae9a089dcaf2239ede354d4/activemodel/lib/active_model/validations/resolve_value.rb#L6 method might be useful.

That method doesn't descend into Hashes and Arrays and tries to call Symbols as methods. That's not what we need for our "options-processor". (Also it isn't yet part of any rails stable version.)
If it wasn't for Rails 5.2 compatibility I would have used deep_transform_values (https://apidock.com/rails/v6.0.0/Hash/deep_transform_values), but that was added in Rails 6. :(

StefSchenkelaars · 2022-09-12T11:24:39Z

lib/active_storage_validations/aspect_ratio_validator.rb

        files = Array.wrap(changes.is_a?(ActiveStorage::Attached::Changes::CreateMany) ? changes.attachables : changes.attachable)

        files.each do |file|
          metadata = Metadata.new(file).metadata
-          next if is_valid?(record, attribute, metadata)
+          next if is_valid?(record, attribute, metadata, options)


This options is an instance variable (or at least it should be). So passing this should be unnecessary. If the unfolding of the options has to be done, do that in the location where you need it (in the private method).

No it is not the instance variable, it's the local variable option which was masked above. Which I will rename to flat_options to make it clearer.

StefSchenkelaars · 2022-09-12T11:34:21Z

lib/active_storage_validations/content_type_validator.rb

+    def authorized_types(options)
+      (Array.wrap(options[:with]) + Array.wrap(options[:in])).compact.map do |type|


This is an instance method but is actually used as a helper. I would really try to make clear that these authorized types are dependent on the record (which they are implicitly since the options depend on the record). I would therefore rewrite this method to something like this:

def authorized_types(record) with = resolve_value(record, options[:with]) in = resolve_value(record, options[:in]) (Array.wrap(with) + Array.wrap(in)).compact.map do |type|

This makes it clear that the authorized types depend on the record and you don't change the original options.

Actually, the authorized types depend on the options (=flat_options now) which have to be passed in. We're using this same pattern in every other validator:

flat_options = unfold_procs(record, self.options, AVAILABLE_CHECKS)

The AVAILABLE_CHECKS constant specifies which keys from the options should be resolved (which, in this case, are %i[with in]). Please check it now after the rename, I think it's much more understandable now.

I don't agree, the authorized types in the end depends on the record (and the options instance variable but that doesn't have to be passed).

You can generate these flat options if you have the options and the record.

I see, I just moved the proc unfolding call into the authorized_types and pass the record inside that instead of the unfolded options.

I find this a lot clearer. Hopefully, you don't mind me pushing on this but I find it important that the lowest level of dependencies (arguments) for each function are used. This way it is harder to screw up, for example by passing the flat_options that were generated with a different record.

gr8bit · 2022-09-14T23:24:48Z

@StefSchenkelaars I commented on and coded towards your suggestions. :) What do you think?

StefSchenkelaars · 2022-09-22T05:38:53Z

I still don't really like the passing of these options all the time but I'm ok if it works.

gr8bit · 2022-09-24T18:14:40Z

I still don't really like the passing of these options all the time but I'm ok if it works.

I'm totally open to suggestions! Please check the validators and find a better/easier way - keep in mind though the options have to be processed on every validation.

StefSchenkelaars

Looks good, some remarks in the same line.

StefSchenkelaars · 2022-09-26T09:33:11Z

lib/active_storage_validations/aspect_ratio_validator.rb

@@ -54,45 +53,45 @@ def validate_each(record, attribute, _value)
    private


-    def is_valid?(record, attribute, metadata)
+    def is_valid?(record, attribute, metadata, flat_options)


This additional depencency is not needed for this method. You can calculate the flat_options in this method since you have the record already.

True, I changed it.

StefSchenkelaars · 2022-09-26T09:37:45Z

lib/active_storage_validations/dimension_validator.rb

          break
        end
      end
    end


-    def is_valid?(record, attribute, file_metadata)
+    def is_valid?(record, attribute, file_metadata, flat_options)


Same here, you already have the record. Less arguments = better :)

Agreed and changed :)

StefSchenkelaars · 2022-09-26T09:39:00Z

lib/active_storage_validations/dimension_validator.rb

-      return if record.errors.added?(attribute, key)
-      record.errors.add(attribute, key, **attrs)
+    def add_error(record, attribute, default_message, **attrs)
+      message = options[:message].presence || default_message


Maybe an interesting possibility is to allow a proc for the message as well. But lets do that in a separate PR if people would like that idea.

gr8bit · 2022-10-04T23:50:20Z

I think this PR could be merged now.

StefSchenkelaars · 2022-10-05T13:48:03Z

@igorkasyanchuk Agree with @gr8bit that it's good for a merge. I think you are the only one who can do that.

igorkasyanchuk · 2022-10-05T14:02:46Z

Great) will do it later today
Thanks all for tremendous amount of work

igorkasyanchuk · 2022-10-05T18:50:10Z

active_storage_validations-1.0.0.gem released

igorkasyanchuk · 2022-10-06T11:39:37Z

looks like one regression #166 if someone can take a look on this

Muhammad Abdullah added 6 commits October 4, 2021 18:48

Allow Procs to be passed as options

a4503c1

Test added for the allow of procs as dynamic values

5b2cc9d

Dimension validator code refactor

0cbf13c

Documentation updated for the Proc

d818ad4

documentation updated

88f4497

Documentation updated

b3bcce0

igorkasyanchuk reviewed Oct 6, 2021

View reviewed changes

gr8bit added 2 commits December 4, 2021 22:15

rename method to avoid confusion with local variables

aab5904

reformat long line

29b0e3f

gr8bit added 8 commits September 10, 2022 20:03

updated documentation

d651d9c

added method to unfold options hash with procs

b6a3f8a

keep info about not unfold procs when traversing

162bbae

fix missing parameter

4dfa0e4

use unfold_procs in all validators

a8347c8

ignore intellij configs

48ea8f5

shorten diff

611fc78

StefSchenkelaars reviewed Sep 12, 2022

View reviewed changes

gr8bit added 3 commits September 15, 2022 01:05

cleanup (no functionality change)

7a3dcab

do not mask options method with local variable

0c9616f

re-added lost renames

804d036

move option proc unfolding

ffc7297

StefSchenkelaars reviewed Sep 26, 2022

View reviewed changes

move option unfolding to the methods actually using these options

a9047b3

igorkasyanchuk merged commit c439869 into igorkasyanchuk:master Oct 5, 2022

		def authorized_types(options)
		(Array.wrap(options[:with]) + Array.wrap(options[:in])).compact.map do \|type\|

#105 Allow Procs for dynamic value usage #135

#105 Allow Procs for dynamic value usage #135

Conversation

codegeek319 commented Oct 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gr8bit commented Feb 4, 2022

igorkasyanchuk commented Feb 6, 2022

igorkasyanchuk commented Feb 6, 2022

igorkasyanchuk commented Feb 6, 2022

gr8bit commented Feb 6, 2022

igorkasyanchuk commented Apr 17, 2022

gr8bit commented Apr 17, 2022

gr8bit commented Sep 10, 2022

igorkasyanchuk commented Sep 11, 2022

StefSchenkelaars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gr8bit Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gr8bit Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gr8bit Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gr8bit commented Sep 14, 2022 • edited Loading

StefSchenkelaars commented Sep 22, 2022

gr8bit commented Sep 24, 2022

StefSchenkelaars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gr8bit commented Oct 4, 2022

StefSchenkelaars commented Oct 5, 2022

igorkasyanchuk commented Oct 5, 2022

igorkasyanchuk commented Oct 5, 2022

igorkasyanchuk commented Oct 6, 2022

gr8bit Sep 14, 2022 •

edited

Loading

gr8bit Sep 14, 2022 •

edited

Loading

gr8bit Sep 14, 2022 •

edited

Loading

gr8bit commented Sep 14, 2022 •

edited

Loading